Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a race condition in simultaneous poetry installations updating the cache #6186

Merged
merged 4 commits into from
Sep 15, 2022

Conversation

zweger
Copy link
Contributor

@zweger zweger commented Aug 17, 2022

If one poetry installation is writing to the cache while a second installer is attempting to read that cache file, the second installation will fail because the in-flight cache file is invalid while it is still being written to by the first process.

This PR resolves this issue by having Poetry write to a temporary file in the cache directory first, and then rename the file after it's written, which is ~atomic.

Pull Request Check List

Resolves: #5142

  • Added tests for changed code.
  • Updated documentation for changed code.

I'm not sure how to test this change, as the conditions which cause this bug to appear are a little hard to reproduce.

@radoering
Copy link
Member

Makes sense to me. I just wonder if it's wise to import a helper function from requests.utils. It's probably not part of the public API, is it? Maybe, we should add "our own" atomic_open() (to poetry.utils.helpers maybe)?

I don't know how to test this either. Maybe, we can go without a test here.

@neersighted What do you think?

@neersighted
Copy link
Member

I'm wondering if #6471 is the better solution -- I agree that we should yoink atomic_open into this repo (with a comment for attribution/licensing) if we go forward with this. However, as either would likely be part of 1.3 exclusively, I think making the OS work for us instead of against us is the better long-term path forward.

@dimbleby
Copy link
Contributor

dimbleby commented Sep 11, 2022

#6471 and this are independent fixes, it would make perfect sense to merge them both

(#6471 relates to files owned by cachecontrol's filecache, this one relates to a file written by poetry regardless of whether caching is enabled or not)

Edit: rather than the race condition with multiple poetry processes that zweger describes, I bet a more common bug that this fixes is: user hits Ctrl-C while part way through a download. Currently this leaves a half-downloaded file in place, if I understand correctly this should stop that from happening.

@neersighted
Copy link
Member

Fair enough, and good point

@zweger
Copy link
Contributor Author

zweger commented Sep 15, 2022

Hi, thanks for the feedback. I've updated this PR to include atomic_open in poetry.utils.helpers. Regarding testing, I will note that while poetry doesn't have a test for this particular situation (concurrent installations / ctrl+c during install), poetry's current tests do at least cover this code.

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/installer Related to the dependency installer impact/changelog Requires a changelog entry kind/refactor Pulls that refactor, or clean-up code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid hashes when running multiple Poetry installs simultaneously
4 participants